feat: implement input sanitization and rate limiting middleware - #479
Merged
ritik4ever merged 1 commit intoJun 25, 2026
Merged
Conversation
|
@edehvictor is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@edehvictor Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughBackend config gains two environment-backed fields. Rate limiting now uses configurable read/write limits and method-based defaults. Campaign payload validation now rejects script and SQL comment patterns and sanitizes accepted strings. New tests cover the middleware and schema behavior. ChangesBackend configuration
Rate limiting middleware
Campaign payload sanitization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/src/validation/schemas.ts (1)
55-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
--SQL-comment heuristic will reject legitimate text.The
--branch matches common prose (e.g., "Project X -- Phase 2", em-dash usage), so valid titles/descriptions are silently rejected with a confusing error. Note that input filtering is not a reliable SQL-injection defense; parameterized queries / an ORM at the persistence layer are. Consider relaxing or removing this heuristic and relying on parameterized queries downstream.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/validation/schemas.ts` at line 55, The SQL comment heuristic in containsSqlComment is too broad because the `--` check rejects legitimate prose and title text. Update the validation in schemas.ts by relaxing or removing the `--`-based match, and keep the check focused on actual comment delimiters like block comments if needed. Ensure the schema rules for the affected fields still validate user input appropriately while relying on parameterized queries or the persistence layer for SQL-injection protection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/index.ts`:
- Around line 65-67: The rate limit constants in index.ts are currently derived
with Number(...), which can turn invalid env values into NaN or 0 and break
throttling. Update the parsing for RATE_LIMIT_WINDOW_MS,
RATE_LIMIT_MAX_REQUESTS, and WRITE_RATE_LIMIT_MAX_REQUESTS to validate that the
env values are positive safe integers, and fall back to the existing defaults
when they are missing or invalid. Keep the fix localized to the rate-limit setup
near the RATE_LIMIT_* constants so the downstream header and limiter logic
always receives valid numbers.
- Around line 117-138: The rate limiting logic in the request handler leaves
expired entries in rateLimitBuckets indefinitely, which can cause unbounded
memory growth. Update the rate-limit path around the existing current/resetAt
handling to remove buckets whose resetAt has passed before computing headers or
updating the count, and consider pruning the key on each request when the window
has expired so stale IP/type entries do not accumulate.
In `@backend/src/rateLimiter.test.ts`:
- Around line 11-17: The rate limiter tests are reusing the same request object
and IP across calls, which causes applyRateLimit to carry over the
rateLimitedProcessed bypass flag and shared bucket state. Update the test setup
around mockReq and the repeated applyRateLimit calls to create a fresh request
object for each simulated HTTP request, and use a unique ip per test/case so
state does not leak between assertions. Reference the applyRateLimit test helper
usage and the beforeEach mockReq initialization when making the change.
In `@backend/src/validation/schemas.ts`:
- Around line 48-53: The sanitizeInput helper currently misses entity escaping
for ampersands and quotes, which lets entity-encoded payloads bypass
containsScriptTag and later render unsafely. Update sanitizeInput to escape "&"
first, then the other special characters, and include escaping for both
quotation marks; keep the change centered on sanitizeInput so the existing
validation flow still works. Also adjust the corresponding expectations in
schemas.test.ts to match the new escaped output.
---
Nitpick comments:
In `@backend/src/validation/schemas.ts`:
- Line 55: The SQL comment heuristic in containsSqlComment is too broad because
the `--` check rejects legitimate prose and title text. Update the validation in
schemas.ts by relaxing or removing the `--`-based match, and keep the check
focused on actual comment delimiters like block comments if needed. Ensure the
schema rules for the affected fields still validate user input appropriately
while relying on parameterized queries or the persistence layer for
SQL-injection protection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85d92105-9e78-4d59-87ee-19527daffe3b
📒 Files selected for processing (5)
backend/src/config.tsbackend/src/index.tsbackend/src/rateLimiter.test.tsbackend/src/validation/schemas.test.tsbackend/src/validation/schemas.ts
Comment on lines
+65
to
+67
| const RATE_LIMIT_WINDOW_MS = Number(process.env.RATE_LIMIT_WINDOW_MS ?? 60000); | ||
| const RATE_LIMIT_MAX_REQUESTS = Number(process.env.RATE_LIMIT_READ_LIMIT ?? process.env.RATE_LIMIT_MAX_REQUESTS ?? 120); | ||
| const WRITE_RATE_LIMIT_MAX_REQUESTS = Number(process.env.RATE_LIMIT_WRITE_LIMIT ?? process.env.WRITE_RATE_LIMIT_MAX_REQUESTS ?? 20); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate env limits before using them.
Number(...) accepts invalid config as NaN or 0, which can make headers emit NaN and effectively bypass throttling. Parse positive safe integers and fall back when invalid.
Suggested fix
-const RATE_LIMIT_WINDOW_MS = Number(process.env.RATE_LIMIT_WINDOW_MS ?? 60000);
-const RATE_LIMIT_MAX_REQUESTS = Number(process.env.RATE_LIMIT_READ_LIMIT ?? process.env.RATE_LIMIT_MAX_REQUESTS ?? 120);
-const WRITE_RATE_LIMIT_MAX_REQUESTS = Number(process.env.RATE_LIMIT_WRITE_LIMIT ?? process.env.WRITE_RATE_LIMIT_MAX_REQUESTS ?? 20);
+function parsePositiveInt(value: string | undefined, fallback: number): number {
+ const parsed = Number(value);
+ return Number.isSafeInteger(parsed) && parsed > 0 ? parsed : fallback;
+}
+
+const RATE_LIMIT_WINDOW_MS = parsePositiveInt(process.env.RATE_LIMIT_WINDOW_MS, 60000);
+const RATE_LIMIT_MAX_REQUESTS = parsePositiveInt(
+ process.env.RATE_LIMIT_READ_LIMIT ?? process.env.RATE_LIMIT_MAX_REQUESTS,
+ 120,
+);
+const WRITE_RATE_LIMIT_MAX_REQUESTS = parsePositiveInt(
+ process.env.RATE_LIMIT_WRITE_LIMIT ?? process.env.WRITE_RATE_LIMIT_MAX_REQUESTS,
+ 20,
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const RATE_LIMIT_WINDOW_MS = Number(process.env.RATE_LIMIT_WINDOW_MS ?? 60000); | |
| const RATE_LIMIT_MAX_REQUESTS = Number(process.env.RATE_LIMIT_READ_LIMIT ?? process.env.RATE_LIMIT_MAX_REQUESTS ?? 120); | |
| const WRITE_RATE_LIMIT_MAX_REQUESTS = Number(process.env.RATE_LIMIT_WRITE_LIMIT ?? process.env.WRITE_RATE_LIMIT_MAX_REQUESTS ?? 20); | |
| function parsePositiveInt(value: string | undefined, fallback: number): number { | |
| const parsed = Number(value); | |
| return Number.isSafeInteger(parsed) && parsed > 0 ? parsed : fallback; | |
| } | |
| const RATE_LIMIT_WINDOW_MS = parsePositiveInt(process.env.RATE_LIMIT_WINDOW_MS, 60000); | |
| const RATE_LIMIT_MAX_REQUESTS = parsePositiveInt( | |
| process.env.RATE_LIMIT_READ_LIMIT ?? process.env.RATE_LIMIT_MAX_REQUESTS, | |
| 120, | |
| ); | |
| const WRITE_RATE_LIMIT_MAX_REQUESTS = parsePositiveInt( | |
| process.env.RATE_LIMIT_WRITE_LIMIT ?? process.env.WRITE_RATE_LIMIT_MAX_REQUESTS, | |
| 20, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/index.ts` around lines 65 - 67, The rate limit constants in
index.ts are currently derived with Number(...), which can turn invalid env
values into NaN or 0 and break throttling. Update the parsing for
RATE_LIMIT_WINDOW_MS, RATE_LIMIT_MAX_REQUESTS, and WRITE_RATE_LIMIT_MAX_REQUESTS
to validate that the env values are positive safe integers, and fall back to the
existing defaults when they are missing or invalid. Keep the fix localized to
the rate-limit setup near the RATE_LIMIT_* constants so the downstream header
and limiter logic always receives valid numbers.
Comment on lines
117
to
+138
| const now = Date.now(); | ||
| const current = rateLimitBuckets.get(key); | ||
|
|
||
| if (!current || now >= current.resetAt) { | ||
| rateLimitBuckets.set(key, { | ||
| count: 1, | ||
| resetAt: now + RATE_LIMIT_WINDOW_MS, | ||
| }); | ||
| return next(); | ||
| let count = 1; | ||
| let resetAt = now + RATE_LIMIT_WINDOW_MS; | ||
|
|
||
| if (current && now < current.resetAt) { | ||
| count = current.count + 1; | ||
| resetAt = current.resetAt; | ||
| } | ||
|
|
||
| if (current.count >= maxRequests) { | ||
| res.setHeader("X-RateLimit-Limit", String(maxRequests)); | ||
| res.setHeader("X-RateLimit-Remaining", String(Math.max(0, maxRequests - count))); | ||
| res.setHeader("X-RateLimit-Reset", String(Math.ceil(resetAt / 1000))); | ||
|
|
||
| if (current && now < current.resetAt && current.count >= maxRequests) { | ||
| const retryAfterSec = Math.max(1, Math.ceil((current.resetAt - now) / 1000)); | ||
| res.setHeader('Retry-After', String(retryAfterSec)); | ||
| throw new AppError('Rate limit exceeded. Please retry shortly.', 429, 'RATE_LIMITED'); | ||
| res.setHeader("Retry-After", String(retryAfterSec)); | ||
| throw new AppError("Rate limit exceeded. Please retry shortly.", 429, "RATE_LIMITED"); | ||
| } | ||
|
|
||
| current.count += 1; | ||
| rateLimitBuckets.set(key, current); | ||
| rateLimitBuckets.set(key, { count, resetAt }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prune expired buckets to avoid unbounded memory growth.
Each distinct IP/type key remains in rateLimitBuckets forever unless the same key returns. A botnet or spoofed-proxy scenario can grow this map without bound.
Suggested fix
+let lastRateLimitSweep = 0;
+
+function sweepExpiredRateLimitBuckets(now: number): void {
+ if (now - lastRateLimitSweep < RATE_LIMIT_WINDOW_MS) return;
+ lastRateLimitSweep = now;
+
+ for (const [bucketKey, bucket] of rateLimitBuckets.entries()) {
+ if (now >= bucket.resetAt) {
+ rateLimitBuckets.delete(bucketKey);
+ }
+ }
+}
+
const key = `${req.ip}:${isWrite ? "write" : "read"}`;
const now = Date.now();
+ sweepExpiredRateLimitBuckets(now);
const current = rateLimitBuckets.get(key);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const now = Date.now(); | |
| const current = rateLimitBuckets.get(key); | |
| if (!current || now >= current.resetAt) { | |
| rateLimitBuckets.set(key, { | |
| count: 1, | |
| resetAt: now + RATE_LIMIT_WINDOW_MS, | |
| }); | |
| return next(); | |
| let count = 1; | |
| let resetAt = now + RATE_LIMIT_WINDOW_MS; | |
| if (current && now < current.resetAt) { | |
| count = current.count + 1; | |
| resetAt = current.resetAt; | |
| } | |
| if (current.count >= maxRequests) { | |
| res.setHeader("X-RateLimit-Limit", String(maxRequests)); | |
| res.setHeader("X-RateLimit-Remaining", String(Math.max(0, maxRequests - count))); | |
| res.setHeader("X-RateLimit-Reset", String(Math.ceil(resetAt / 1000))); | |
| if (current && now < current.resetAt && current.count >= maxRequests) { | |
| const retryAfterSec = Math.max(1, Math.ceil((current.resetAt - now) / 1000)); | |
| res.setHeader('Retry-After', String(retryAfterSec)); | |
| throw new AppError('Rate limit exceeded. Please retry shortly.', 429, 'RATE_LIMITED'); | |
| res.setHeader("Retry-After", String(retryAfterSec)); | |
| throw new AppError("Rate limit exceeded. Please retry shortly.", 429, "RATE_LIMITED"); | |
| } | |
| current.count += 1; | |
| rateLimitBuckets.set(key, current); | |
| rateLimitBuckets.set(key, { count, resetAt }); | |
| let lastRateLimitSweep = 0; | |
| function sweepExpiredRateLimitBuckets(now: number): void { | |
| if (now - lastRateLimitSweep < RATE_LIMIT_WINDOW_MS) return; | |
| lastRateLimitSweep = now; | |
| for (const [bucketKey, bucket] of rateLimitBuckets.entries()) { | |
| if (now >= bucket.resetAt) { | |
| rateLimitBuckets.delete(bucketKey); | |
| } | |
| } | |
| } | |
| const key = `${req.ip}:${isWrite ? "write" : "read"}`; | |
| const now = Date.now(); | |
| sweepExpiredRateLimitBuckets(now); | |
| const current = rateLimitBuckets.get(key); | |
| let count = 1; | |
| let resetAt = now + RATE_LIMIT_WINDOW_MS; | |
| if (current && now < current.resetAt) { | |
| count = current.count + 1; | |
| resetAt = current.resetAt; | |
| } | |
| res.setHeader("X-RateLimit-Limit", String(maxRequests)); | |
| res.setHeader("X-RateLimit-Remaining", String(Math.max(0, maxRequests - count))); | |
| res.setHeader("X-RateLimit-Reset", String(Math.ceil(resetAt / 1000))); | |
| if (current && now < current.resetAt && current.count >= maxRequests) { | |
| const retryAfterSec = Math.max(1, Math.ceil((current.resetAt - now) / 1000)); | |
| res.setHeader("Retry-After", String(retryAfterSec)); | |
| throw new AppError("Rate limit exceeded. Please retry shortly.", 429, "RATE_LIMITED"); | |
| } | |
| rateLimitBuckets.set(key, { count, resetAt }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/index.ts` around lines 117 - 138, The rate limiting logic in the
request handler leaves expired entries in rateLimitBuckets indefinitely, which
can cause unbounded memory growth. Update the rate-limit path around the
existing current/resetAt handling to remove buckets whose resetAt has passed
before computing headers or updating the count, and consider pruning the key on
each request when the window has expired so stale IP/type entries do not
accumulate.
Comment on lines
+11
to
+17
| beforeEach(() => { | ||
| nextCalled = false; | ||
| headers = {}; | ||
| mockReq = { | ||
| ip: "127.0.0.1", | ||
| method: "GET", | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use fresh request objects for each simulated HTTP request.
applyRateLimit sets rateLimitedProcessed on the request, so the second and third calls reuse the bypass flag and never increment the bucket. Also use a unique IP per test to avoid leaking module-level bucket state between cases.
Suggested fix
describe("Rate Limiter Middleware", () => {
let mockReq: Partial<Request>;
let mockRes: Partial<Response>;
let nextCalled: boolean;
let headers: Record<string, string>;
+ let testIpCounter = 0;
+ let testIp: string;
beforeEach(() => {
nextCalled = false;
headers = {};
+ testIp = `127.0.0.${++testIpCounter}`;
mockReq = {
- ip: "127.0.0.1",
+ ip: testIp,
method: "GET",
};
@@
it("should enforce rate limiting and throw 429 when limit is exceeded", () => {
- mockReq.method = "POST";
+ const makePostReq = () => ({ ip: testIp, method: "POST" }) as Request;
const middleware = applyRateLimit(2); // Set limit to 2 for testing
// First request
- middleware(mockReq as Request, mockRes as Response, next);
+ middleware(makePostReq(), mockRes as Response, next);
expect(nextCalled).toBe(true);
// Second request
nextCalled = false;
- middleware(mockReq as Request, mockRes as Response, next);
+ middleware(makePostReq(), mockRes as Response, next);
expect(nextCalled).toBe(true);
// Third request - should exceed limit
nextCalled = false;
expect(() => {
- middleware(mockReq as Request, mockRes as Response, next);
+ middleware(makePostReq(), mockRes as Response, next);
}).toThrow(/Rate limit exceeded/);Also applies to: 53-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/rateLimiter.test.ts` around lines 11 - 17, The rate limiter tests
are reusing the same request object and IP across calls, which causes
applyRateLimit to carry over the rateLimitedProcessed bypass flag and shared
bucket state. Update the test setup around mockReq and the repeated
applyRateLimit calls to create a fresh request object for each simulated HTTP
request, and use a unique ip per test/case so state does not leak between
assertions. Reference the applyRateLimit test helper usage and the beforeEach
mockReq initialization when making the change.
Comment on lines
+48
to
+53
| function sanitizeInput(val: string): string { | ||
| return val | ||
| .replace(/</g, "<") | ||
| .replace(/>/g, ">") | ||
| .replace(/\//g, "/"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical
Incomplete HTML escaping creates an entity-based XSS bypass
sanitizeInput escapes <, >, and / but fails to escape &. Consequently, entity-encoded payloads like &#60;script&#62; bypass the containsScriptTag check (which only matches literal <script) and are persisted unescaped. When rendered, & decodes back to <, allowing script execution.
Additionally:
- The
&replacement must occur before escaping other entities to prevent double-encoding issues. - Quotes (
",') are also missing for attribute context safety. - Updates to
schemas.test.ts(Lines 42-43) are required to reflect the corrected output.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 48-50: Avoid hand-rolled HTML escaping (replacing characters with HTML entities); use a vetted encoder/sanitizer such as DOMPurify or sanitize-html.
Context: val
.replace(/</g, "<")
.replace(/>/g, ">")
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(manual-sanitization-typescript)
[warning] 48-49: Avoid hand-rolled HTML escaping (replacing characters with HTML entities); use a vetted encoder/sanitizer such as DOMPurify or sanitize-html.
Context: val
.replace(/</g, "<")
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(manual-sanitization-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/validation/schemas.ts` around lines 48 - 53, The sanitizeInput
helper currently misses entity escaping for ampersands and quotes, which lets
entity-encoded payloads bypass containsScriptTag and later render unsafely.
Update sanitizeInput to escape "&" first, then the other special characters, and
include escaping for both quotation marks; keep the change centered on
sanitizeInput so the existing validation flow still works. Also adjust the
corresponding expectations in schemas.test.ts to match the new escaped output.
This was referenced Jun 29, 2026
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request addresses two assigned tasks under the backend scope:
Input Sanitization via Zod Schema (Issue #211):
titleanddescriptionpayload properties.<script>) or SQL comment sequences (--,/*,*/).schemas.test.ts.Rate Limiting Middleware (Issue #210):
RATE_LIMIT_WINDOW_MS,RATE_LIMIT_READ_LIMIT,RATE_LIMIT_WRITE_LIMIT) are dynamically configurable via environment variables.X-RateLimit-Limit,X-RateLimit-Remaining, andX-RateLimit-Resetheaders to all responses.Retry-Afterheader.rateLimiter.test.ts.. General Build/Compilation Fix:
config.tswherecontractIdandsorobanRpcUrlwere referenced on the export object but were not declared.Verification
Closes #206
Closes #207
Closes #210
Closes #211
Summary by CodeRabbit
New Features
Bug Fixes
Tests